Add commit ID to version output #370

mgoerens · 2023-07-10T12:31:15Z

add a "build" make target, which injects the git commit ID using ldflags
add the commit ID to the version output
add the possibility to get the version and commit information in a JSON format

Example output:

[mgoerens@mgoerens-thinkpadp1gen3 chart-verifier]$ ./chart-verifier version
chart-verifier v1.12.0 <commit: 1c1d51406d0241cbf9683b8dd731eb4bcb706dec>
[mgoerens@mgoerens-thinkpadp1gen3 chart-verifier]$ ./chart-verifier version --as-data
{"version":"1.12.0","commit":"1c1d51406d0241cbf9683b8dd731eb4bcb706dec"}[mgoerens@mgoerens-thinkpadp1gen3 chart-verifier]$

komish · 2023-07-10T14:10:34Z

@mgoerens thanks for this. The linter is grumpy ^ https://github.com/redhat-certification/chart-verifier/actions/runs/5508530811/jobs/10039939259?pr=370#step:7:154

mgoerens · 2023-07-10T15:31:23Z

This should be fixed now.

mgoerens · 2023-07-10T15:31:41Z

Fixes #370

komish · 2023-07-10T15:49:07Z

Makefile

+.PHONY: build
+build:
+	go build -ldflags "-X 'github.com/redhat-certification/chart-verifier/cmd.CommitIDLong=$(COMMIT_ID_LONG)'"
+


Lets use the existing bin target instead of having a separate build target. Also add this to bin_win for now.

I'll collapse those targets at some point in the future, but for now, we can duplicate the logic in both places.

komish · 2023-07-10T15:51:05Z

cmd/version.go

 func init() {
 	rootCmd.AddCommand(NewVersionCmd())
 }

+// CommitIDLong contains the commit ID the binary was build on. It is populated at build time by ldflags.
+// If you're running from a local debugger it will show an empty commit ID.
+var CommitIDLong string


Let's preseed this with a value of unknown

komish · 2023-07-10T15:52:44Z

cmd/version.go

 func NewVersionCmd() *cobra.Command {
 	cmd := &cobra.Command{
 		Use:   "version",
 		Short: "print the chart-verifier version information",
+		Long:  "git commit information for this particular binary build is included at build time and can be accessed by this command",


We can leave the long description out

komish · 2023-07-10T16:02:38Z

cmd/version.go

+			return err
+		}
+		fmt.Fprint(out, string(marshalledVersion))
+	} else {


Let's remove this else here. Instead, return nil in the if block after line 56.

E.g. (disregard pseudocode names, just demonstrating structure)

func runVersion(...) ... { if asData { printAsDataHopefullySuccessfully() return nil } printThingsNormally() return nil } }

Ok, I was thinking about this and was balanced. Good that you have a strong opinion on this, that helps making a decision :)

cmd/version.go

komish · 2023-07-10T16:09:52Z

@mgoerens Left a couple of comments.

The CI complaint is an interesting way. Effectively what it's implying is that the chart-verifier version output is used in report generation, and since we've changed its output, we're breaking our fixtures. Normally, I'd say we can update our fixtures, however I think for this case, I think we want to make sure we're backwards compatible with the reports because changing that has some hefty implications.

Please let me know if you need any guidance with this. I'd also recommend you are rebased off of main at any given time before submitting your PR, just to rule out any differences between your branch and main when it comes to CI. It can be a little finicky with differences between a PR and main.

komish · 2023-07-10T16:24:27Z

Quick note, the problem is two-fold.

Your CLI version is no longer using the library at pkg/chartverifier/version.GetVersion. We need to make sure we're using this because this is shared across both the CLI and the library use cases.
Our tests are asserting that the version in generated reports matches the version string in the binary that produces them. We're expecting this to look a specific way ("vX.Y.Z") because this is then normalized and used for comparison against the report.

With this in mind, you can take this one of two ways, I think:

a) The easiest thing to do is just to add the commit information as a flag value. In other words, leave chart-verifier version untouched. Add a --commit flag that spits out the commit. Leave --as-data as you have it. This is backwards compatible but also adds the new features, even if isn't the best UX. You might also introduce pkg/chartverifier/version.GetCommit to facilitate this.

b) Update the e2e tests[1] to comply with expectations. Maybe it can use the --as-data value as a dictionary and be more reliable? This is trickier because these can be finicky.

I'm perfectly fine with both options. Your choice!

[1]https://github.com/redhat-certification/chart-verifier/blob/main/tests/tests/functional/chart_test.py#L57-L86

mgoerens · 2023-07-10T16:36:32Z

The code does still make use of pkg/chartverifier/version.GetVersion

include apiversion "github.com/redhat-certification/chart-verifier/pkg/chartverifier/version"

[...]

var Version = VersionContext{
	Version: apiversion.GetVersion(),
	Commit:  CommitIDLong,
}

I ran make test prior to submitting the PR and was happy to get a green light. I didn't realize their are also a set of python tests ?

I'll look at option a) and b) in details tomorrow.

komish · 2023-07-10T16:46:02Z

The code does still make use of pkg/chartverifier/version.GetVersion

Aha, missed this. Thanks for pointing it out!

mgoerens · 2023-07-17T16:21:16Z

All right. Pipeline passes. Though I still don't manage to reproduce it locally, the error seemed to be coming from the lack of newline in the output of version --as-data. I can't understand why, but only some docker tests were failing due to that, and not always the same were failing depending on which one were run. For instance in this pipeline where I commented out the first docker test + all podman and tarball tests, only the 5th and 6th test pass (out of seven). If I run all tests only the first docker test passes, and following docker tests fail (all podman and tarball tests pass regardless). Go figure ... Besides, I cannot reproduce this behavior on a fresh debian instance.

So I added a newline and now all tests pass.

Full list of changes (from commit message):

- use ldflags to injects the git commit ID at build time
- add the commit ID to the version output
- add the possibility to get the version and commit information in a
  JSON format
- adapt the tests to make use of the JSON output
- adapt Dockerfile to make use of the make bin target
- fix path to chart used to test the built docker image in buildandtest
  script (moved as part of !271) and make use of the JSON output

Reg. the last point: the "build-and-test" script is, as the name suggests, comprised of two parts: first it builds the container image, then it tests the image using verify on a known chart. The script is called with the --build-only flag in the pipeline i.e. the image is not tested using this script.

There are multiple issues with the "test" part which makes this script not usable as-is (prior to this PR):

the chart used for testing has been moved to another location (easy fix)
the verify command uses a -l flag which isn't supported anymore (easy fix)
the verify command requires access to a Kubernetes cluster. Currently there is no way to provide Kubernetes credentials to the script (fixable, with some effort)

But before putting further effort into fixing the "test" part of this script, it appears to me like we don't need this script to be able to test a docker image anymore. The pipeline takes care of that in a next step (actually running some extensive tests). AFAICT this script is never called without the --build-only flag in our testing / release process (and anyway it would not have worked, even prior to this PR).

My suggestion is to not fix this part and to consider dropping the "test" part of the buildandtest script altogether in another PR.

mgoerens · 2023-07-17T16:23:20Z

Opened #378 for the buildandtest issue

- use ldflags to injects the git commit ID at build time - add the commit ID to the version output - add the possibility to get the version and commit information in a JSON format - adapt the tests to make use of the JSON output - adapt Dockerfile to make use of the make bin target - fix path to chart used to test the built docker image in buildandtest script (moved as part of !271) and make use of the JSON output close redhat-certification#362 Signed-off-by: mgoerens <[email protected]>

komish

/lgtm

Thanks @mgoerens!

mgoerens force-pushed the add_commit_id_to_version branch from 1c1d514 to 65d9898 Compare July 10, 2023 13:13

mgoerens force-pushed the add_commit_id_to_version branch from 65d9898 to 1cb9515 Compare July 10, 2023 15:30

komish linked an issue Jul 10, 2023 that may be closed by this pull request

Add commit information to version output of chart-verifier #362

Closed

komish reviewed Jul 10, 2023

View reviewed changes

cmd/version.go Show resolved Hide resolved

mgoerens force-pushed the add_commit_id_to_version branch from 1cb9515 to 7bd9203 Compare July 10, 2023 16:39

mgoerens force-pushed the add_commit_id_to_version branch 10 times, most recently from 4dcd90a to 4d8a725 Compare July 17, 2023 15:24

mgoerens force-pushed the add_commit_id_to_version branch from 4d8a725 to e73878a Compare July 18, 2023 15:37

komish approved these changes Jul 18, 2023

View reviewed changes

komish merged commit 40ba87f into redhat-certification:main Jul 18, 2023

mgoerens deleted the add_commit_id_to_version branch July 31, 2023 08:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add commit ID to version output #370

Add commit ID to version output #370

mgoerens commented Jul 10, 2023

komish commented Jul 10, 2023

mgoerens commented Jul 10, 2023

mgoerens commented Jul 10, 2023

komish Jul 10, 2023

komish Jul 10, 2023

komish Jul 10, 2023

komish Jul 10, 2023

mgoerens Jul 10, 2023

komish commented Jul 10, 2023

komish commented Jul 10, 2023 •

edited

Loading

mgoerens commented Jul 10, 2023

komish commented Jul 10, 2023

mgoerens commented Jul 17, 2023

mgoerens commented Jul 17, 2023

komish left a comment

Add commit ID to version output #370

Add commit ID to version output #370

Conversation

mgoerens commented Jul 10, 2023

komish commented Jul 10, 2023

mgoerens commented Jul 10, 2023

mgoerens commented Jul 10, 2023

komish Jul 10, 2023

Choose a reason for hiding this comment

komish Jul 10, 2023

Choose a reason for hiding this comment

komish Jul 10, 2023

Choose a reason for hiding this comment

komish Jul 10, 2023

Choose a reason for hiding this comment

mgoerens Jul 10, 2023

Choose a reason for hiding this comment

komish commented Jul 10, 2023

komish commented Jul 10, 2023 • edited Loading

mgoerens commented Jul 10, 2023

komish commented Jul 10, 2023

mgoerens commented Jul 17, 2023

mgoerens commented Jul 17, 2023

komish left a comment

Choose a reason for hiding this comment

komish commented Jul 10, 2023 •

edited

Loading